feat(Cssbuilder): improve performance use StringBuilder #6799
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideThis PR refactors the CssBuilder implementation for better performance by switching to StringBuilder, removes legacy AddStyle methods, standardizes attribute value conversion, updates component code to use the new AddClass API for styling, and aligns unit tests with these changes. Class diagram for refactored CssBuilderclassDiagram
class CssBuilder {
- StringBuilder _builder
- bool _hasConent
+ CssBuilder(string? value)
+ CssBuilder AddClass(string? value)
+ CssBuilder AddClass(string value, bool when)
+ CssBuilder AddClass(string value, Func<bool> when)
+ CssBuilder AddClassFromAttributes(IDictionary<string, object>? additionalAttributes)
+ CssBuilder AddStyleFromAttributes(IDictionary<string, object>? additionalAttributes)
+ string? Build()
}
class StringBuilder
CssBuilder --> StringBuilder
Class diagram for affected component style propertiesclassDiagram
class ShieldBadge {
- string? StyleString
}
class Captcha {
- string? StyleString
- string? FailedStyle
}
class Drawer {
- string? StyleString
- string? DrawerStyleString
- bool IsVertical
}
class AutoFill {
- string? PlaceHolderStyleString
}
class Card {
- string? HeaderStyleString
}
class EditorForm {
- string? FormStyleString
}
class BootstrapLabel {
- string? StyleString
}
class Logout {
- string? AvatarStyleString
}
class NavbarGroup {
- string? StyleString
}
class Toolbar {
- string? StyleString
}
class ValidateForm {
- string? StyleString
}
ShieldBadge --> CssBuilder
Captcha --> CssBuilder
Drawer --> CssBuilder
AutoFill --> CssBuilder
Card --> CssBuilder
EditorForm --> CssBuilder
BootstrapLabel --> CssBuilder
Logout --> CssBuilder
NavbarGroup --> CssBuilder
Toolbar --> CssBuilder
ValidateForm --> CssBuilder
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves performance of the CssBuilder class by replacing the internal List with StringBuilder for CSS generation. The change eliminates multiple string concatenations in favor of a more efficient single-buffer approach.
- Replace List with StringBuilder in CssBuilder for better performance
- Remove unused AddStyle methods to simplify the API surface
- Update component code to use AddClass instead of removed AddStyle methods
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Utils/CssBuilder.cs | Core performance improvement - replaced List with StringBuilder and removed AddStyle methods |
| test/UnitTest/Utils/CssBuilderTest.cs | Removed tests for deleted AddStyle methods |
| test/UnitTest/Components/DrawerTest.cs | Updated test assertions to match new CSS variable output format |
| src/BootstrapBlazor/Components/ValidateForm/ValidateForm.razor.cs | Changed AddStyle to AddClass call |
| src/BootstrapBlazor/Components/Toolbar/Toolbar.razor.cs | Changed AddStyle to AddClass call |
| src/BootstrapBlazor/Components/Navbar/NavbarGroup.razor.cs | Changed AddStyle to AddClass call |
| src/BootstrapBlazor/Components/Logout/Logout.razor.cs | Changed AddStyle to AddClass call |
| src/BootstrapBlazor/Components/Label/BootstrapLabel.razor.cs | Changed AddStyle to AddClass call |
| src/BootstrapBlazor/Components/EditorForm/EditorForm.razor.cs | Changed AddStyle to AddClass call |
| src/BootstrapBlazor/Components/Drawer/Drawer.razor.cs | Changed AddStyle to AddClass calls and improved logic with IsVertical property |
| src/BootstrapBlazor/Components/Card/Card.razor.cs | Changed AddStyle to AddClass call with null check |
| src/BootstrapBlazor/Components/Captcha/Captcha.razor.cs | Changed AddStyle to AddClass calls and improved LINQ usage |
| src/BootstrapBlazor/Components/Badge/ShieldBadge.razor.cs | Changed AddStyle to AddClass calls |
| src/BootstrapBlazor/Components/AutoFill/AutoFill.razor.cs | Changed AddStyle to AddClass call with culture-invariant formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+16
to
+17
| private readonly StringBuilder _builder = new(); | ||
| private bool _hasConent; |
There was a problem hiding this comment.
The field name _hasConent has a spelling error. It should be _hasContent.
| if (!string.IsNullOrEmpty(value)) | ||
| { | ||
| _builder.Append(value); | ||
| _hasConent = true; |
There was a problem hiding this comment.
The field name _hasConent has a spelling error. It should be _hasContent.
| if (!string.IsNullOrEmpty(value)) stringBuffer.Add(value); | ||
| if (!string.IsNullOrEmpty(value)) | ||
| { | ||
| if (_hasConent) |
There was a problem hiding this comment.
The field name _hasConent has a spelling error. It should be _hasContent.
| } | ||
| else | ||
| { | ||
| _hasConent = true; |
There was a problem hiding this comment.
The field name _hasConent has a spelling error. It should be _hasContent.
| /// </summary> | ||
| /// <returns>string</returns> | ||
| public string? Build() => stringBuffer.Count > 0 ? string.Join(" ", stringBuffer) : null; | ||
| public string? Build() => _hasConent ? _builder.ToString() : null; |
There was a problem hiding this comment.
The field name _hasConent has a spelling error. It should be _hasContent.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Utils/CssBuilder.cs:17` </location>
<code_context>
{
- private readonly List<string> stringBuffer;
+ private readonly StringBuilder _builder = new();
+ private bool _hasConent;
/// <summary>
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in variable name '_hasConent'.
Rename '_hasConent' to '_hasContent' for consistency.
Suggested implementation:
```csharp
private bool _hasContent;
```
If `_hasConent` is referenced elsewhere in the file (e.g., in methods or properties), those references should also be updated to `_hasContent`.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Utils/CssBuilderTest.cs:24-25` </location>
<code_context>
+ Assert.DoesNotContain("--bb-drawer-width", cut.Markup);
}
[Fact]
</code_context>
<issue_to_address>
**suggestion (testing):** No tests for edge cases with empty, null, or whitespace-only strings in AddClass after refactor.
Add tests to confirm AddClass ignores empty, null, and whitespace-only strings without affecting output.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6799 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 739 739
Lines 31743 31748 +5
Branches 4467 4464 -3
=========================================
+ Hits 31743 31748 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #6798
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refactor CssBuilder to improve performance by replacing the internal string list with a StringBuilder and unifying style handling, and update all component usage accordingly
Enhancements:
Tests: